Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ignored type variable bound warning in type quote pattern #18199

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Jul 13, 2023

See the diff in tests/neg-macros/quote-type-variable-no-inference.check

@nicolasstucki nicolasstucki force-pushed the fix-ignored-type-quote-pattern-type-variable-bound-inference-warning branch from 63e03ab to e9d18bd Compare July 13, 2023 13:47
@nicolasstucki nicolasstucki marked this pull request as ready for review July 14, 2023 07:28
@nicolasstucki nicolasstucki requested a review from smarter July 14, 2023 07:29
@@ -3,7 +3,8 @@
| ^
| Ignored bound <: Double
|
| Consider defining bounds explicitly `'{ type t <: Int & Double; ... }`
| This kind of pattern will be supported with SIP-53.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the message mention the experimental import or link to the doc page on it if we have one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this message can only be shown in an experimental context. We do not need this extra warning.

The only way to have this way to get code like this is using one of the two addition of SIP-53. Those are caught just before this check in https://github.com/lampepfl/dotty/pull/18199/files#diff-2eae364f82b9f4896588e75bfe0bdda905455d0d76136914f7a8909005048bf2R174-R177

@nicolasstucki nicolasstucki force-pushed the fix-ignored-type-quote-pattern-type-variable-bound-inference-warning branch 2 times, most recently from c9e89c3 to b698052 Compare July 14, 2023 12:00
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

.addMode(Mode.Pattern)
.withOwner(quoteOwner(ctx))

/** Owner of the quoted pattern */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the documentation accurate since it also does something when we're not in a quoted pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the documentation and name of the definition

| Ignored bound <: Double
|
| Consider defining bounds explicitly:
| '{ type t <: Int & Double; ... }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could take advantage of #17561 to automatically propose a rewrite that does that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am considering doing that. The complication is finding the position where this needs to be inserted. It would be also be a bit more challenging if we consider handling two of these errors at the same time. I plan to try this in the near future.

case '{ $_ : F[t, t]; () } => // warn // error
case '{ type u <: Int & Double; $_ : F[u, u] } =>

type F[x <: Int, y <: Double]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an example involving an F-bounds (like type Foo[T <: Comparable[T]])?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added am example in tests/neg-macros/quote-type-variable-no-inference-3.scala

@smarter smarter assigned nicolasstucki and unassigned smarter Jul 14, 2023
@nicolasstucki nicolasstucki force-pushed the fix-ignored-type-quote-pattern-type-variable-bound-inference-warning branch from b698052 to c8a7eee Compare July 19, 2023 07:32
@nicolasstucki nicolasstucki requested a review from smarter July 19, 2023 08:52
@nicolasstucki nicolasstucki merged commit dde69ce into scala:main Jul 20, 2023
@nicolasstucki nicolasstucki deleted the fix-ignored-type-quote-pattern-type-variable-bound-inference-warning branch July 20, 2023 12:39
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants